-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Color by attribute #148
base: main
Are you sure you want to change the base?
Color by attribute #148
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…f default attributes when fetching
Hi @andy-sweet and @aganders3, This PR is a bit bigger and contains the feature to color cells based on provided attributes. In short, 1) extra columns in the Considerations for review:
Thanks! |
It looks like you're creating another ragged array in the top-level of the Zarr bundle as I agree packing it all into |
Hi @aganders3, thanks for having a look! Yes, that is correct, What do you mean with "making |
Ahh, I see. I missed this distinction and thought the attributes were still interleaved. Never mind my suggestion then, I think this achieves the same thing! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still making my way through this, but left lots of minor comments and one or two more important ones. Will try to take another look tomorrow.
Qualitatively, I noticed a minor performance hit, but not enough of one to block this feature which seems really useful!
@@ -11,16 +11,26 @@ const config = { | |||
// When opening the viewer, or refreshing the page, the viewer will revert to the following default dataset | |||
data:{ | |||
// Default dataset URL (must be publically accessible) | |||
default_dataset: "https://public.czbiohub.org/royerlab/zoo/Zebrafish/tracks_zebrafish_bundle.zarr/" | |||
// default_dataset: "https://public.czbiohub.org/royerlab/zoo/Zebrafish/tracks_zebrafish_bundle.zarr/" | |||
default_dataset: "https://public.czbiohub.org/royerlab/zoo/misc/tracks_drosophila_attributes_norm_bundle.zarr/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be the following?
default_dataset: "https://public.czbiohub.org/royerlab/zoo/misc/tracks_drosophila_attributes_norm_bundle.zarr/" | |
default_dataset: "https://public.czbiohub.org/royerlab/zoo/misc/tracks_zebrafish_displ_norm_bundle.zarr/" |
@@ -117,6 +117,13 @@ intracktive convert --csv_file path/to/tracks.csv --add_radius | |||
|
|||
Or use `intracktive convert --help` for the documentation on the inputs and outputs | |||
|
|||
**ToDo: explain how to add attributes** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this TODO still need to be done?
@@ -223,17 +225,26 @@ export default function App() { | |||
const getPoints = async (time: number) => { | |||
console.debug("fetch points at time %d", time); | |||
const data = await trackManager.fetchPointsAtTime(time); | |||
// console.log('data shape:', data.length, 'attributes shape:', attributes.length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe combine this with the debug log below? Or remove it.
const [inputDropdownValue, setInputDropdownValue] = useState<string | undefined>(options[0]?.name); | ||
const [value, setValue] = useState<AutocompleteValue<Option, false, false, false> | null>(options[0] || null); | ||
|
||
// useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be removed? If not add a comment to the code to explain why it might be useful in the future.
<DropdownMenu | ||
open={open} | ||
anchorEl={anchorEl} | ||
// onClose={noop} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still needed?
// const numPoints = positions.count / numberOfValuesPerPoint; | ||
// console.log('numPoints in getAtt:',numPoints, positions.count, numberOfValuesPerPoint) | ||
// console.log('positions:',positions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// const numPoints = positions.count / numberOfValuesPerPoint; | |
// console.log('numPoints in getAtt:',numPoints, positions.count, numberOfValuesPerPoint) | |
// console.log('positions:',positions) |
const quadrant = x + y * 2 + z * 4; // | ||
attributeVector.push(quadrant); // color based on XY coordinates (4 groups) | ||
} else { | ||
attributeVector.push(1); // default to constant color if event type not recognized |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe an error should be logged in this case?
@@ -153,6 +157,28 @@ export class TrackManager { | |||
return array; | |||
} | |||
|
|||
async fetchAttributessAtTime(timeIndex: number, attributeIndex: number): Promise<Float32Array> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
async fetchAttributessAtTime(timeIndex: number, attributeIndex: number): Promise<Float32Array> { | |
async fetchAttributesAtTime(timeIndex: number, attributeIndex: number): Promise<Float32Array> { |
|
||
export const numberOfDefaultColorByOptions = DEFAULT_DROPDOWN_OPTIONS.length; | ||
// Initialize the mutable dropdown options with the default options | ||
export const dropDownOptions: Option[] = [...DEFAULT_DROPDOWN_OPTIONS]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't made a drop-down in SDS, but the code here feels problematic on a first look because this is a mutable array of Options that is effectively used as React state.
For react state, the typical pattern is to just create a new array each time one is needed rather than mutate a single instance.
|
||
return ( | ||
<div> | ||
<InputDropdown |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the SDS docs, I'd expect to see DropDown
here instead of InputDropdown
and DropdownMenu
.
But I've never actually used any of those, so probably ignore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the zarr storage format and fetching (when needed) is as reasonable as prior choices we've made, so I don't have any feedback related to that or the Python conversion code/tools.
My main comment is about the how you handle the state related to the attribute drop-down box value changing. I previously left some related comments, but I made those a bit more tangible by trying some changes that I think simplify things and also fix some bugs: https://github.com/royerlab/inTRACKtive/compare/color-by-attribute...andy-sweet/refactor-attr-options?expand=1#diff-8e6c274c4241821e809d16fcd06bc109ba41bb7a0583f9ad766ed92bb8230671R8
I don't think you should blindly merge those changes, but I think the approach there is easier to follow and importantly does not make the library code depend on definitions from React components.
{ name: "y-position", label: 2, type: "continuous", action: "calculate", numCategorical: undefined }, | ||
{ name: "z-position", label: 3, type: "continuous", action: "calculate", numCategorical: undefined }, | ||
{ name: "sign(x-pos)", label: 4, type: "categorical", action: "calculate", numCategorical: 2 }, | ||
{ name: "quadrants", label: 4, type: "categorical", action: "calculate", numCategorical: 8 }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming these two options should not have the same label.
{ name: "quadrants", label: 4, type: "categorical", action: "calculate", numCategorical: 8 }, | |
{ name: "quadrants", label: 5, type: "categorical", action: "calculate", numCategorical: 8 }, |
@@ -24,13 +26,17 @@ import { Track } from "@/lib/three/Track"; | |||
import { PointSelector, PointSelectionMode } from "@/lib/PointSelector"; | |||
import { ViewerState } from "./ViewerState"; | |||
import { numberOfValuesPerPoint } from "./TrackManager"; | |||
import { Option, dropDownOptions } from "@/components/leftSidebar/DynamicDropdown"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a rule, we tried to avoid make the code in lib
depend on definitions in components
, so we can define the core logic of the library and rendering independently of React.
I closed #147, and started a new one on this topic:
This PR will implement the option to color the cells based on some attribute. The attribute could be any column added to the
tracks.csv
, or calculated features from the coordinates (think of velocity, direction, etc.)Implemented:
tracks.csv
into Zarr Store.zattrs
trackMaterial
colormaps)Bugs:
Example datasets:
Example of coloring based on z-coordinate: